-
Notifications
You must be signed in to change notification settings - Fork 69
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add safeIdentifier
and safeObjectProperty
helpers
#391
Conversation
190fea8
to
d5d0cea
Compare
Now that we found that bug re: prototype inheritance, it looks like it might have been relied on because one of the fixtures fails now. That suggest to me that we'll need to update something in the generator. Would you mind helping me locate what?fixtures fails now. That suggest to me that we'll need to update something in the generator. Would you mind helping me locate what needs to be updated? Is it somewhere in |
safeIdentifier
and safeObjectProperty
helperssafeIdentifier
and safeObjectProperty
helpers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks really nice! Two comments though:
packages/protobuf-bench/README.md
Outdated
@@ -10,5 +10,5 @@ server would usually do. | |||
|
|||
| code generator | bundle size | minified | compressed | | |||
|---------------------|------------------------:|-----------------------:|-------------------:| | |||
| protobuf-es | 86,785 b | 36,950 b | 9,642 b | | |||
| protobuf-es | 87,019 b | 36,999 b | 9,692 b | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you look into this? We should not increase the bundle size. I know it's tedious for 50 bytes compressed, but we don't make any logical changes here... I'm happy to help looking at the bundle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I definitely need help: I cannot for the life of me determine what's going on. see https://github.com/bufbuild/protobuf-es/tree/dimitri/test-safe-names and note that in the last commit the bundle.js file didn't change, despite the size being reported to be different. I also know it's not from the exports or the test files (by process of elimination).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You didn't see changes because gather() is called twice, once for protobuf-es, once for protobuf-javascript, but you always write to bundle.js, overwriting the protobuf-es bundle output with the protobuf-javascript output, which does not change with this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I merged in main for #411.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
safeIdentifier
and safeObjectProperty
helperssafeIdentifier
and safeObjectProperty
helpers
This release contains the following: ## Enhancements * Add `safeIdentifier` and `safeObjectProperty` helpers by @dimitropoulos in #391. **Full Changelog**: v1.1.1...v1.2.0
It came up in connectrpc/connect-query-es#19 that we need to export the functionality of safely modifying names for identifiers.
This PR adds
safeIdentifier
andsafeObjectProperty
helpers which do exactly that. All prior behavior should be the same (except for these functions now being isolated, tested, and exported).